Fix duckdb build failure #11059
Fix duckdb build failure #11059
Conversation
facebook-github-bot
added
the
CLA Signed
✅ Deploy Preview for meta-velox canceled.
|
CMakeLists.txt
Outdated
| @@ -50,7 +50,7 @@ if(DEFINED ENV{INSTALL_PREFIX}) | |||
| list(APPEND CMAKE_PREFIX_PATH "$ENV{INSTALL_PREFIX}") | |||
| # Allow installed package headers to be picked up before brew/system package | |||
| # headers | |||
| include_directories(BEFORE "$ENV{INSTALL_PREFIX}/include") | |||
| include_directories("$ENV{INSTALL_PREFIX}/include") | |||
There was a problem hiding this comment.
Please also remove the comment regarding the before ordering above.
Just curious, since we already have CMAKE_PREFIX_PATH to include $INSTALL_PREFIX, do we need include_directories("$ENV{INSTALL_PREFIX}/include") at all?
There was a problem hiding this comment.
Please also remove the comment regarding the
beforeordering above.Just curious, since we already have
CMAKE_PREFIX_PATHto include$INSTALL_PREFIX, do we needinclude_directories("$ENV{INSTALL_PREFIX}/include")at all?
Good catch. I actually think include_directories("$ENV{INSTALL_PREFIX}/include") is not needed. I will try it on a new repo tomorrow.
There was a problem hiding this comment.
@zuyu, @yingsu00 as described in the comment, we want INSTALL_PREFIX headers to appear before brew headers in the include order. So we need this.
The assumption here was that users either installed dependencies from Bundling or used this script. Mixing both is not a supported case.
But I see that on MacOS, we always bundle DuckDB. And likely DuckDB prefers its own fmt version.
We can bundle DuckDB before this line and fix the issue.
There was a problem hiding this comment.
Building DuckDB before the include statement works. Thank you for the explanation, @majetideepak!
index 8b0d3a67c..f31f91b69 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -48,9 +48,6 @@ endif()
if(DEFINED ENV{INSTALL_PREFIX})
message(STATUS "Dependency install directory set to: $ENV{INSTALL_PREFIX}")
list(APPEND CMAKE_PREFIX_PATH "$ENV{INSTALL_PREFIX}")
- # Allow installed package headers to be picked up before brew/system package
- # headers
- include_directories(BEFORE "$ENV{INSTALL_PREFIX}/include")
endif()
list(PREPEND CMAKE_MODULE_PATH "${PROJECT_SOURCE_DIR}/CMake"
@@ -192,6 +189,18 @@ if(${VELOX_BUILD_PYTHON_PACKAGE})
set(VELOX_ENABLE_SPARK_FUNCTIONS ON)
endif()
+if(${VELOX_ENABLE_DUCKDB})
+ set_source(DuckDB)
+ resolve_dependency(DuckDB)
+endif()
+
+if(DEFINED ENV{INSTALL_PREFIX})
+ # Allow installed package headers to be picked up before brew/system package
+ # headers
+ include_directories(BEFORE "$ENV{INSTALL_PREFIX}/include")
+endif()
+
+
# We look for OpenSSL here to cache the result enforce the version across our
# dependencies.
find_package(OpenSSL REQUIRED)
@@ -413,11 +423,6 @@ else()
endif()
resolve_dependency(glog)
-if(${VELOX_ENABLE_DUCKDB})
- set_source(DuckDB)
- resolve_dependency(DuckDB)
-endif()
-
set_source(fmt)
resolve_dependency(fmt 9.0.0)
yingsu00
force-pushed
the
before
branch
2 times, most recently
from
816bc69 to
8210260
Compare
Upgrade FBOS dependencies to 2024.09.16.00 installs all dependencies in deps-install subfolder, and set the include directory to it in front of other include directories. This caused duckdb build failed with "error: use of undeclared identifier 'FMT_SNPRINTF'". This commit removes the BEFORE keyword to fix the problem.
|
I have a PR here with the proper fix |
|
Closing for |
Improve dependency install and build (#10920) installs all dependencies in
deps-install subfolder, and set the include directory to it in front of
other include directories. This caused duckdb build failed with "error:
use of undeclared identifier 'FMT_SNPRINTF'". This commit removes the
BEFORE keyword to fix the problem.
Fixes #11058